Switch to k8s client-go #15486

daxmc99 · 2020-11-05T19:20:11Z

Fixes https://github.com/sourcegraph/sourcegraph/issues/11804

This is needed as part of https://github.com/sourcegraph/sourcegraph/issues/13878 since without this we cannot use a current client-go (this client is ~2 years old)

sourcegraph-bot · 2020-11-05T23:47:18Z

Notifying subscribers in CODENOTIFY files for diff aedcaf7...263717f.

Notify	File(s)
@keegancsmith	internal/endpoint/endpoint.go

uwedeportivo

LGTM, please add keegan or somebody from cloud as a reviewer. endpoint.go is used to discover zoekt service endpoints so it's rather important :-)

uwedeportivo · 2020-11-05T23:57:03Z

cmd/frontend/internal/app/debugproxies/scanner.go

+// namespace returns the namespace the pod is currently running in
+// this is done because the k8s client we previously used set the namespace
+// when the client was created, the official k8s client does not
+func namespace() string {


can you please rewrite this a little to log the error if it happens.

codecov · 2020-11-06T02:39:20Z

Codecov Report

Merging #15486 (817d48d) into main (aedcaf7) will increase coverage by 1.57%.
The diff coverage is 11.26%.

@@            Coverage Diff             @@
##             main   #15486      +/-   ##
==========================================
+ Coverage   50.89%   52.47%   +1.57%     
==========================================
  Files        1736     1614     -122     
  Lines       86759    80634    -6125     
  Branches     7874     7168     -706     
==========================================
- Hits        44156    42311    -1845     
+ Misses      38711    34617    -4094     
+ Partials     3892     3706     -186

Flag	Coverage Δ		*Carryforward flag
go	`52.55% <11.26%> (+2.81%)`	⬆️
integration	`28.76% <ø> (-1.56%)`	⬇️	Carriedforward from f669e1b
storybook	`27.29% <ø> (-3.28%)`	⬇️	Carriedforward from f669e1b
typescript	`52.27% <ø> (-1.38%)`	⬇️	Carriedforward from f669e1b
unit	`34.34% <ø> (-1.04%)`	⬇️	Carriedforward from f669e1b

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
internal/endpoint/endpoint.go	`34.95% <0.00%> (-6.00%)`	⬇️
cmd/frontend/internal/app/debugproxies/scanner.go	`29.72% <25.00%> (-8.30%)`	⬇️
client/web/src/tree/Directory.tsx	`11.11% <0.00%> (-58.89%)`	⬇️
client/web/src/tree/SingleChildTreeLayer.tsx	`0.00% <0.00%> (-57.98%)`	⬇️
...t/web/src/search/input/toggles/CopyQueryButton.tsx	`14.28% <0.00%> (-57.15%)`	⬇️
client/web/src/repo/blob/panel/BlobPanel.tsx	`42.85% <0.00%> (-54.29%)`	⬇️
client/web/src/util/path.tsx	`50.00% <0.00%> (-50.00%)`	⬇️
client/web/src/insights/InsightsNavLink.tsx	`50.00% <0.00%> (-50.00%)`	⬇️
...terprise/code-monitoring/CodeMonitoringNavItem.tsx	`50.00% <0.00%> (-50.00%)`	⬇️
client/web/src/search/QuickLinks.tsx	`60.00% <0.00%> (-40.00%)`	⬇️
... and 1061 more

uwedeportivo

LGTM

keegancsmith · 2020-11-09T09:20:55Z

I'll review the code soon, but at a high level this pulls in a crap load of transitive dependencies. We used to use client-go, and constantly ran into dependency issues due to how much it depends on and how they change.

Why do you need client-go? Does this simple client not solve it for you?

keegancsmith

Approving. Some comments inline. This looks like it works, my main concern is that client-go is a seven tonne gorilla that I'd rather avoid in our dependency tree.

keegancsmith · 2020-11-09T09:22:21Z

cmd/frontend/internal/app/debugproxies/scanner.go

@@ -102,21 +85,23 @@ func (cs *clusterScanner) runEventLoop() {
 // watchEndpointEvents uses the k8s watch API operation to watch for endpoint events. Spins forever unless an error
 // occurs that would necessitate creating a new watcher. The caller will then call again creating the new watcher.
 func (cs *clusterScanner) watchEndpointEvents() (bool, error) {
-	watcher, err := cs.client.Watch(context.Background(), cs.client.Namespace(), new(corev1.Endpoints))
+
+	// TODO(Dax): Rewrite this to used NewSharedInformerFactory from k8s/client-go


you may have to look at older versions of this repo, but I believe we used to use informer back when this package used client-go.

keegancsmith · 2020-11-09T09:23:02Z

cmd/frontend/internal/app/debugproxies/scanner.go

 	for {
-		var eps corev1.Endpoints
-		eventType, err := watcher.Next(&eps)
+		event := <-watcher.ResultChan()


you should be able to do for event := range watcher.ResultChan()

cmd/frontend/internal/app/debugproxies/scanner.go

keegancsmith · 2020-11-09T09:29:19Z

internal/endpoint/endpoint.go

-	defer watcher.Close()
+func inform(client v1.EndpointsInterface, m *Map, u *k8sURL) error {
+
+	// TODO(Dax): We shouldn't use watch directly, use an informer here


FYI if watch is simpler, we should just use that. I think Informer may be simple, I can't remember exactly :) But Informer is designed to help scale / simplify access, but we only care about simplicity not scale in this case.

keegancsmith · 2020-11-09T09:29:36Z

internal/endpoint/endpoint.go

-		eventType, err := watcher.Next(&endpoints)
-		if err != nil {
-			return errors.Wrap(err, "watcher.Next")
+		event := <-watcher.ResultChan()


same as the previous code snippet, you can for loop over the chan

keegancsmith · 2020-11-09T09:30:00Z

internal/endpoint/endpoint.go

+		e := event.Object
+		endpoints, ok := e.(*corev1.Endpoints)
+		if !ok {
+			return errors.Wrap(err, "object from watcher is not an endpoint")


have you manually tested this? Should be fine, but this sort of runtime reflection can slip through the cracks.

keegancsmith · 2020-11-09T09:31:07Z

internal/endpoint/endpoint.go

+		return "default"
+	}
+	return ns
+}


same comments from the other place this was copied from.

daxmc99 · 2020-11-09T18:29:08Z

After your comment about this pulling in more dependencies, I checked the frontend binary size before and after this change:

before 68M 
after 117M

This is a 72% increase in our binary size which feels excessive for what is essentially a refactor to use a more well-documented client.

I knew that client-go might have some issues with other dependencies but there are no conflicts and the post 1.17.0 versions are fairly easy to update.

Co-authored-by: Keegan Carruthers-Smith <[email protected]>

daxmc99 · 2020-11-10T06:46:17Z

I also spent some time today trying to upgrade https://github.com/ericchiang/k8s but the protobuf generation no longer works

keegancsmith · 2020-11-10T14:58:17Z

After your comment about this pulling in more dependencies, I checked the frontend binary size before and after this change:
before 68M 
after 117M
This is a 72% increase in our binary size which feels excessive for what is essentially a refactor to use a more well-documented client.

I knew that client-go might have some issues with other dependencies but there are no conflicts and the post 1.17.0 versions are fairly easy to update.

Yeah I am guessing this has calmed down vs a few years ago.

I also spent some time today trying to upgrade https://github.com/ericchiang/k8s but the protobuf generation no longer works

@daxmc99 you wanna pair tomorrow to try and sort this out? I'm sure making our current dep work on modern protobuf will make others happy.

daxmc99 · 2020-12-02T04:21:26Z

The cluster QA tests are currently running via a pure bash implementation.
The results of our pairing session are available here:
ericchiang/k8s#128

The upstream project has been archived though so we should avoid new uses of ericchiang/k8s where possible.

keegancsmith · 2020-12-02T12:09:28Z

The cluster QA tests are currently running via a pure bash implementation.

Cool. So just using kubectl?

The upstream project has been archived though so we should avoid new uses of `ericchiang/k8s` where possible.

Should we consider forking and maintaining it? It seems like not that much effort to be honest and in our interests to keep up to date. I imagine keeping it up to date will be easier than updating deps on the offical k8s client :)

daxmc99 · 2020-12-03T17:55:14Z

Cool. So just using kubectl?

Correct

Should we consider forking and maintaining it?

I still hesitate to use a non-official client. I wonder if the way forward here is actually to reduce scope further and drop a client altogether and build our own basic client ( https://github.com/cilium/cilium/blob/master/pkg/k8s/client.go#L93:6). We intentionally use the k8s API for very little and our current dependency actually blocks us from using newer versions of protobuf.

…

On Wed, Dec 2, 2020 at 5:09 AM Keegan Carruthers-Smith < ***@***.***> wrote: > The cluster QA tests are currently running via a pure bash > implementation. Cool. So just using kubectl? > The upstream project has been archived though so we should avoid > new uses of `ericchiang/k8s` where possible. Should we consider forking and maintaining it? It seems like not that much effort to be honest and in our interests to keep up to date. I imagine keeping it up to date will be easier than updating deps on the offical k8s client :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/sourcegraph/sourcegraph/pull/15486#issuecomment-737190465>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHS5HJQSSHOJH7VQAHDDBLDSSYVANANCNFSM4TLYSKZQ> .

keegancsmith · 2020-12-03T18:38:35Z

> Should we consider forking and maintaining it? I still hesitate to use a non-official client. I wonder if the way forward here is actually to reduce scope further and drop a client altogether and build our own basic client

I'm all for doing that. Our use of the API is super tiny. Its watch and get on endpoints... and I think maybe a get on svc. Thats it. In fact I learnt how to do what I wanted by running kubectl in verbose mode anyways, which just output the urls to hit. Sounds like fun :)

arussellsaw · 2021-02-12T11:21:47Z

👋 i'm gonna pick this up to resolve conflicts & merge, as it's blocking some work for secret encryption. The GCloud KMS library depends on a newer protobuf version, and won't compile. I've pulled these changes into my local branch and everything looks fine. context: https://sourcegraph.slack.com/archives/CHPC7UX16/p1613123508259400

daxmc99 marked this pull request as ready for review November 5, 2020 23:46

daxmc99 requested review from uwedeportivo and bobheadxi and removed request for uwedeportivo November 5, 2020 23:46

uwedeportivo approved these changes Nov 5, 2020

View reviewed changes

daxmc99 requested a review from keegancsmith November 6, 2020 00:01

bobheadxi approved these changes Nov 6, 2020

View reviewed changes

uwedeportivo approved these changes Nov 6, 2020

View reviewed changes

keegancsmith approved these changes Nov 9, 2020

View reviewed changes

daxmc99 mentioned this pull request Nov 9, 2020

Distribution: 2020.11.03 - Amazing Otter #14630

Closed

52 tasks

daxmc99 and others added 12 commits November 9, 2020 18:49

Drop github.com/ericchiang/k8s for the official client

4d7a7eb

Remove github.com/ericchiang/k8s from endpoints

dbf854e

small fixes

e1f628e

tests pass

f142a5a

dependency update

2902722

update ns func

b9dca01

mod

1f5afb4

tidy

b9efd6f

pin dependency

720eaf6

Update cmd/frontend/internal/app/debugproxies/scanner.go

003e962

Co-authored-by: Keegan Carruthers-Smith <[email protected]>

Update cmd/frontend/internal/app/debugproxies/scanner.go

e321993

Co-authored-by: Keegan Carruthers-Smith <[email protected]>

comments

817d48d

daxmc99 force-pushed the dax/use_client-go branch from c0145db to 817d48d Compare November 10, 2020 06:35

merge main

263717f

arussellsaw merged commit ac37bb2 into main Feb 12, 2021

arussellsaw deleted the dax/use_client-go branch February 12, 2021 11:41

sourcegraph-bot mentioned this pull request Feb 12, 2021

Update Sourcegraph Docker insiders images sourcegraph/deploy-sourcegraph#1826

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to k8s client-go #15486

Switch to k8s client-go #15486

daxmc99 commented Nov 5, 2020 •

edited

Loading

sourcegraph-bot commented Nov 5, 2020 •

edited

Loading

uwedeportivo left a comment

uwedeportivo Nov 5, 2020

daxmc99 Nov 6, 2020

codecov bot commented Nov 6, 2020 •

edited

Loading

uwedeportivo left a comment

keegancsmith commented Nov 9, 2020

keegancsmith left a comment

keegancsmith Nov 9, 2020

keegancsmith Nov 9, 2020

keegancsmith Nov 9, 2020

keegancsmith Nov 9, 2020

keegancsmith Nov 9, 2020

keegancsmith Nov 9, 2020

daxmc99 commented Nov 9, 2020 •

edited

Loading

daxmc99 commented Nov 10, 2020

keegancsmith commented Nov 10, 2020

daxmc99 commented Dec 2, 2020

keegancsmith commented Dec 2, 2020 via email

daxmc99 commented Dec 3, 2020 via email

keegancsmith commented Dec 3, 2020 via email

arussellsaw commented Feb 12, 2021

Switch to k8s client-go #15486

Switch to k8s client-go #15486

Conversation

daxmc99 commented Nov 5, 2020 • edited Loading

sourcegraph-bot commented Nov 5, 2020 • edited Loading

uwedeportivo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 6, 2020 • edited Loading

Codecov Report

uwedeportivo left a comment

Choose a reason for hiding this comment

keegancsmith commented Nov 9, 2020

keegancsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxmc99 commented Nov 9, 2020 • edited Loading

daxmc99 commented Nov 10, 2020

keegancsmith commented Nov 10, 2020

daxmc99 commented Dec 2, 2020

keegancsmith commented Dec 2, 2020 via email

daxmc99 commented Dec 3, 2020 via email

keegancsmith commented Dec 3, 2020 via email

arussellsaw commented Feb 12, 2021

daxmc99 commented Nov 5, 2020 •

edited

Loading

sourcegraph-bot commented Nov 5, 2020 •

edited

Loading

codecov bot commented Nov 6, 2020 •

edited

Loading

daxmc99 commented Nov 9, 2020 •

edited

Loading